Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use stable metric for const eval limit instead of current terminator-based logic #106227

Merged
merged 19 commits into from
Jan 29, 2023
Merged

Use stable metric for const eval limit instead of current terminator-based logic #106227

merged 19 commits into from
Jan 29, 2023

Conversation

bryangarza
Copy link
Contributor

@bryangarza bryangarza commented Dec 28, 2022 •
edited
Loading

This patch adds a MirPass that inserts a new MIR instruction ConstEvalCounter to any loops and function calls in the CFG. This instruction is used during Const Eval to count against the const_eval_limit, and emit the StepLimitReached error, replacing the current logic which uses Terminators only.

The new method of counting loops and function calls should be more stable across compiler versions (i.e., not cause crates that compiled successfully before, to no longer compile when changes to the MIR generation/optimization are made).

Also see: #103877

@rustbot
Copy link
Collaborator

rustbot commented Dec 28, 2022

r? @Nilstrieb

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 28, 2022
@bryangarza
Copy link
Contributor Author

r? @pnkfelix

@rustbot rustbot assigned pnkfelix and unassigned Noratrieb Dec 28, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bryangarza
Copy link
Contributor Author

bryangarza commented Dec 29, 2022 •
edited
Loading

Remaining:

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bryangarza bryangarza changed the title (WIP) Create stable metric to measure long computation in Const Eval Create stable metric to measure long computation in Const Eval Dec 29, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bryangarza bryangarza changed the title Create stable metric to measure long computation in Const Eval Use stable metric for const eval limit instead of current terminator-based logic Dec 29, 2022
@bryangarza
Copy link
Contributor Author

@rustbot ready

@bryangarza bryangarza marked this pull request as ready for review December 29, 2022 23:45
@rustbot
Copy link
Collaborator

rustbot commented Dec 29, 2022

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

@rustbot
Copy link
Collaborator

rustbot commented Dec 29, 2022

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bryangarza
Copy link
Contributor Author

@rustbot ready

@bryangarza
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 24, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Jan 24, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jan 24, 2023

📌 Commit bdb815a has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 24, 2023
@bors
Copy link
Contributor

bors commented Jan 29, 2023

⌛ Testing commit bdb815a with merge 3cdd019...

@bors
Copy link
Contributor

bors commented Jan 29, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 3cdd019 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 29, 2023
@bors bors merged commit 3cdd019 into rust-lang:master Jan 29, 2023
@rustbot rustbot added this to the 1.69.0 milestone Jan 29, 2023
This was referenced Jan 29, 2023
Open
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3cdd019): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.3% [0.3%, 0.4%] 3
Regressions ❌
(secondary)		 0.8% [0.5%, 1.0%] 9
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.3% [0.3%, 0.4%] 3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 2.3% [2.3%, 2.3%] 1
Regressions ❌
(secondary)		 2.0% [1.8%, 2.2%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 2.3% [2.3%, 2.3%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.4% [-2.4%, -2.4%] 1
All ❌✅ (primary) - - 0

@rustbot rustbot added the perf-regression Performance regression. label Jan 29, 2023
@cjgillot cjgillot mentioned this pull request Jan 29, 2023
Merged
@oli-obk
Copy link
Contributor

oli-obk commented Jan 30, 2023

This small regression is expected. We do some extra work for all CTFE code to insert the new statements. Compared to not doing any work before, even if a very small change, it is still visible on some tests.

@pnkfelix
Copy link
Member

I agree with @oli-obk 's comment above

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Jan 30, 2023
@tedinski tedinski mentioned this pull request Feb 3, 2023
Closed
4 tasks
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Feb 9, 2023
@bors
Auto merge of rust-lang#106227 - bryangarza:ctfe-limit, r=oli-obk
ba7bb62 
Use stable metric for const eval limit instead of current terminator-based logic

This patch adds a `MirPass` that inserts a new MIR instruction `ConstEvalCounter` to any loops and function calls in the CFG. This instruction is used during Const Eval to count against the `const_eval_limit`, and emit the `StepLimitReached` error, replacing the current logic which uses Terminators only.

The new method of counting loops and function calls should be more stable across compiler versions (i.e., not cause crates that compiled successfully before, to no longer compile when changes to the MIR generation/optimization are made).

Also see: rust-lang#103877
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 10, 2023
@bors
Auto merge of rust-lang#106227 - bryangarza:ctfe-limit, r=oli-obk
17369f3 
Use stable metric for const eval limit instead of current terminator-based logic

This patch adds a `MirPass` that inserts a new MIR instruction `ConstEvalCounter` to any loops and function calls in the CFG. This instruction is used during Const Eval to count against the `const_eval_limit`, and emit the `StepLimitReached` error, replacing the current logic which uses Terminators only.

The new method of counting loops and function calls should be more stable across compiler versions (i.e., not cause crates that compiled successfully before, to no longer compile when changes to the MIR generation/optimization are made).

Also see: rust-lang#103877
@qinheping qinheping mentioned this pull request Mar 9, 2023
Merged
4 tasks
@pnkfelix pnkfelix mentioned this pull request Apr 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmiasko tmiasko tmiasko left review comments

@oli-obk oli-obk oli-obk left review comments

@RalfJung RalfJung RalfJung left review comments

@compiler-errors compiler-errors Awaiting requested review from compiler-errors

Assignees

@pnkfelix pnkfelix

@oli-obk oli-obk

Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.69.0
Development

Successfully merging this pull request may close these issues.